Skip to content

add 2fa backup codes app#1171

Merged
MariusBluem merged 3 commits into
masterfrom
2fa-backup-codes
Sep 5, 2016
Merged

add 2fa backup codes app#1171
MariusBluem merged 3 commits into
masterfrom
2fa-backup-codes

Conversation

@ChristophWurst

@ChristophWurst ChristophWurst commented Aug 29, 2016

Copy link
Copy Markdown
Member

integrates https://github.com/ChristophWurst/twofactor_backupcodes and fixes #1108

  • App is always enabled, so users can create backup codes even before an admin randomly enables/enforces a 2FA provider
  • Does not enforce 2FA is this app is the only active 2FA provider
  • Does not show up in the list of selectable 2FA providers
  • Adds a "Use backup code" link underneath the 2FA selection page and 2FA challenge page

TODO:

  • Unit/integration tests for the twofactor_backupcodes app
  • Fix/add tests for the changed server classes

bildschirmfoto von 2016-08-29 19-13-25

@nextcloud/designers any idea how we could make the settings dialog prettier? See the screenshots in https://github.com/ChristophWurst/twofactor_backupcodes for how it looks now.

@mention-bot

Copy link
Copy Markdown

@ChristophWurst, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bantu, @icewind1991 and @DeepDiver1975 to be potential reviewers

</div>
<a class="two-factor-cancel" <?php print_unescaped($_['logout_attribute']); ?>><?php p($l->t('Cancel log in')) ?></a>
<?php if (!is_null($_['backupProvider'])): ?>
<a class="two-factor-cancel" href="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.TwoFactorChallenge.showChallenge',

@ChristophWurst ChristophWurst Aug 29, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • TODO: rename class

@ChristophWurst

Copy link
Copy Markdown
Member Author
/usr/local/bin/phpunit --configuration phpunit-autotest.xml --exclude-group DB --log-junit autotest-results-sqlite.xml  
PHP Parse error:  syntax error, unexpected 'class' (T_CLASS), expecting identifier (T_STRING) or variable (T_VARIABLE) or '{' or '$' in /drone/src/github.com/nextcloud/server/tests/Core/Command/Config/App/DeleteConfigTest.php on line 46
[info] build failed (exit code 255)

unrelated?

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 2, 2016
@ChristophWurst

Copy link
Copy Markdown
Member Author

@rullzer @LukasReschke @nicohauger @zkwoob @Alphakilo @jancborchardt @Mar1u5 review and test this please.

The personal settings dialog definitely needs some UI/UX enhancements, that I'd like to postpone to follow-up PRs as this one is already rather big.

@ChristophWurst

Copy link
Copy Markdown
Member Author

Does PHPUnit automatically find the test of the newly added app or do I have to register them somewhere?

@MorrisJobke

Copy link
Copy Markdown
Member
$ NOCOVERAGE=true TEST_SELECTION=DB ./autotest.sh sqlite
Using PHP executable /usr/bin/php
Parsing all files in lib/public for the presence of @since or @deprecated on each method...

Using database oc_autotest
Setup environment for sqlite testing on local storage ...
Installing ....
Nextcloud is not installed - only a limited number of commands are available
creating sqlite db
Nextcloud was successfully installed
Testing with sqlite ...
No coverage
/usr/local/bin/phpunit --configuration phpunit-autotest.xml --group DB --log-junit autotest-results-sqlite.xml  
PHPUnit 4.8.24 by Sebastian Bergmann and contributors.

Runtime:    PHP 5.6.22-0+deb8u1
Configuration:  /drone/src/github.com/nextcloud/server/tests/phpunit-autotest.xml
Warning:    Deprecated configuration setting "strict" used

........S....................................................   61 / 4240 (  1%)
PHP Fatal error:  Call to a member function getUpdater() on null in /drone/src/github.com/nextcloud/server/lib/private/Files/View.php on line 695
[info] build failed (exit code 255)

@ChristophWurst

Copy link
Copy Markdown
Member Author

let me try to reproduce the failing tests locally…

@ChristophWurst

Copy link
Copy Markdown
Member Author

@MorrisJobke what are the requirements/preconditions to run all tests locally? I get Tests: 4716, Assertions: 12160, Errors: 1359, Failures: 13, Skipped: 286. :-(

@MorrisJobke

Copy link
Copy Markdown
Member

@MorrisJobke what are the requirements/preconditions to run all tests locally?

drone exec

(see http://readme.drone.io/devs/cli/)

Best is also to remove the running tests from .drone.yml this speeds up the test execution ;)

@ChristophWurst

Copy link
Copy Markdown
Member Author

stack trace:

Call to a member function getUpdater() on null in /server/lib/private/Files/View.php on line 696
#0  OC\Files\View->unlink() called at [/server/lib/private/Cache/File.php:146]
#1  OC\Cache\File->remove() called at [/server/tests/lib/Cache/FileCacheTest.php:91]
#2  Test\Cache\FileCacheTest->tearDown() called at [phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:805]
#3  PHPUnit_Framework_TestCase->runBare() called at [phar:///usr/local/bin/phpunit/phpunit/Framework/TestResult.php:612]
#4  PHPUnit_Framework_TestResult->run() called at [phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:724]
#5  PHPUnit_Framework_TestCase->run() called at [phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:747]
#6  PHPUnit_Framework_TestSuite->run() called at [phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:747]
#7  PHPUnit_Framework_TestSuite->run() called at [phar:///usr/local/bin/phpunit/phpunit/TextUI/TestRunner.php:440]
#8  PHPUnit_TextUI_TestRunner->doRun() called at [phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:149]
#9  PHPUnit_TextUI_Command->run() called at [phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:100]
#10 PHPUnit_TextUI_Command::main() called at [/usr/local/bin/phpunit:548]

* add backup codes app unit tests
* add integration tests for the backup codes app
@rullzer

rullzer commented Sep 5, 2016

Copy link
Copy Markdown
Member

Some dark magic. Seems something was not cleaned up properly or so which caused the whole thing to break down.

CI seems a lot happier now ;)

@ChristophWurst

Copy link
Copy Markdown
Member Author

Thank you very much @rullzer 🚀

@rullzer

rullzer commented Sep 5, 2016

Copy link
Copy Markdown
Member

CI happy.
Tested in combination with TOTP and worked like a charm.
Code looks good! LGTM!

@MariusBluem

Copy link
Copy Markdown
Member

Works 👍

@MariusBluem MariusBluem merged commit f8eb7be into master Sep 5, 2016
@MariusBluem MariusBluem deleted the 2fa-backup-codes branch September 5, 2016 10:17
<licence>agpl</licence>
<author>Christoph Wurst</author>
<version>1.0.0</version>
<namespace>TwoFactor_BackupCodes</namespace>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, no underscores in PSR-4 Namespaces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2FA: let users create and authenticate via backup codes

7 participants